Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use zero byte reads for incoming TLS frames #30863

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Mar 12, 2021

  • We recently did some work to reduce the amount of memory allocated by SslStream on idle reads so take advantage of it here by using a new StreamPipeReader feature that makes zero byte reads on the underlying Stream.

You can see some of that work here:

The scenario is 5000 connections sending websocket pings from both server and client:
These are GC Heap sizes:

5000 NO TLS - 68,967,016 B

Before:

5000 TLS - 310,459,272 B

image

After:

5000 TLS - 119,175,792

image

SslStream is still holding onto a 4K buffer but this is a big improvement. I want to get it much closer to the non-TLS connections.

cc @geoffkizer @stephentoub @wfurt

- We recently did some work to reduce the amount of memory allocated by SslStream on idle reads so take advantage of it here by using a new StreamPipeReader feature that makes zero byte reads on the underlying Stream.
@stephentoub
Copy link
Member

Glad to see this having the desired effect.

Should this in some way respect the WaitForDataBeforeAllocatingBuffer setting at the transport layer? Or if not because it's the wrong layer, if such a setting was important there, is one important here?

@davidfowl
Copy link
Member Author

Should this in some way respect the WaitForDataBeforeAllocatingBuffer setting at the transport layer? Or if not because it's the wrong layer, if such a setting was important there, is one important here?

Definitely possible to respect this setting but it would need more public API to flow the setting via a feature as part of the transport layer or have an https specific setting (it'd be a layering violation to flow this existing setting directly into kestrel). I didn't want to do either as part of this PR because it requires a new API and I didn't measure any performance difference is basic scenarios.

@davidfowl
Copy link
Member Author

I filed this to consider adding a new API #30877.

@davidfowl davidfowl merged commit 1e19c6f into main Mar 12, 2021
@davidfowl davidfowl deleted the davidfowl/better-tls branch March 12, 2021 16:18
@BrennanConroy BrennanConroy added this to the 6.0-preview3 milestone Mar 12, 2021
@geoffkizer
Copy link
Contributor

Great to see this validate the SslStream work.

@ghost
Copy link

ghost commented Mar 13, 2021

Hi @geoffkizer. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants